Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[16.0][MIG]: web_edit_user_filter: Migration to 16.0 #3007

Open
wants to merge 15 commits into
base: 16.0
Choose a base branch
from

Conversation

anusriNPS
Copy link
Contributor

Migrating web_edit_user_filter to 16.0

@anusriNPS anusriNPS marked this pull request as draft December 2, 2024 08:12
tarteo and others added 14 commits December 2, 2024 09:29
[FIX] Readme

[FIX] Lint

[ADD] Tests

[IMP] UI/UX

[IMP] Hide popover when other is opened

[IMP] Add item to roadmap
Added translation using Weblate (Chinese (Simplified))

Translated using Weblate (Chinese (Simplified))

Currently translated at 100.0% (3 of 3 strings)

Translation: web-12.0/web-12.0-web_edit_user_filter
Translate-URL: https://translation.odoo-community.org/projects/web-12-0/web-12-0-web_edit_user_filter/zh_CN/

Added translation using Weblate (Croatian)

Translated using Weblate (Croatian)

Currently translated at 66.7% (2 of 3 strings)

Translation: web-12.0/web-12.0-web_edit_user_filter
Translate-URL: https://translation.odoo-community.org/projects/web-12-0/web-12-0-web_edit_user_filter/hr/

Added translation using Weblate (Portuguese)

Translated using Weblate (Portuguese)

Currently translated at 100.0% (3 of 3 strings)

Translation: web-12.0/web-12.0-web_edit_user_filter
Translate-URL: https://translation.odoo-community.org/projects/web-12-0/web-12-0-web_edit_user_filter/pt/
has to explicit check for type of value !== null as the typeof null is object
https://i.stack.imgur.com/FzI1R.png
Added translation using Weblate (Danish)

Translated using Weblate (Danish)

Currently translated at 100.0% (3 of 3 strings)

Translation: web-12.0/web-12.0-web_edit_user_filter
Translate-URL: https://translation.odoo-community.org/projects/web-12-0/web-12-0-web_edit_user_filter/da/
Added translation using Weblate (Spanish)

Translated using Weblate (Spanish)

Currently translated at 100.0% (3 of 3 strings)

Translation: web-12.0/web-12.0-web_edit_user_filter
Translate-URL: https://translation.odoo-community.org/projects/web-12-0/web-12-0-web_edit_user_filter/es/

Added translation using Weblate (French)

Translated using Weblate (French)

Currently translated at 100.0% (3 of 3 strings)

Translation: web-12.0/web-12.0-web_edit_user_filter
Translate-URL: https://translation.odoo-community.org/projects/web-12-0/web-12-0-web_edit_user_filter/fr/

Added translation using Weblate (Catalan)

Translated using Weblate (Catalan)

Currently translated at 100.0% (3 of 3 strings)

Translation: web-12.0/web-12.0-web_edit_user_filter
Translate-URL: https://translation.odoo-community.org/projects/web-12-0/web-12-0-web_edit_user_filter/ca/
Added translation using Weblate (Portuguese (Brazil))

Translated using Weblate (Portuguese (Brazil))

Currently translated at 100.0% (3 of 3 strings)

Translation: web-12.0/web-12.0-web_edit_user_filter
Translate-URL: https://translation.odoo-community.org/projects/web-12-0/web-12-0-web_edit_user_filter/pt_BR/
When clicking to edit a filter in search view, the _process_filters
function will receive the faceID argument as a string, which will
be compared to facet.groupId that is an Integer.

Therefore we should use == operator when comparing this value to
allow type coercion from JS.
@anusriNPS anusriNPS force-pushed the 16.0-mig-web_edit_user_filter branch 7 times, most recently from bb1ccf2 to 8643376 Compare December 3, 2024 10:13
@anusriNPS anusriNPS marked this pull request as ready for review December 3, 2024 10:23
@anusriNPS
Copy link
Contributor Author

Verified Scenarios:

  1. User able to make a filter as favorite and when tries to click on popover of favorite, it unpacks the filter which is stored as favorite
  2. Similarly, user able to mark groupBy as favorite and able to unpack when click on the popover of the favorite.
  3. When both filter and groupby is marked as favorite, both groupby and filter values are unpacked.
  4. Filter/GroupBy facet on click shows popover and user able to remove filter/groupBy values from popover.
  5. When a new favorite is added, user need not reload the page in order to unpack filter/groupBy from favorites. Warning which is available in 14.0 is no longer needed, so removed it.

Unverified Scenarios:

  1. In unpack_filter function, there is a branch deals with tentative filter/groupBy which could not be verified as not able to understand the feature from README file or in general.
    So, I would like to know how this scenario could be verified? Any suggestions would be helpful to verify it.

Comment on lines 18 to 20
for i, res_filter in enumerate(res):
# Add the field 'facet' to the result
res[i]["facet"] = filters.filtered(lambda f: f.id == res_filter["id"]).facet
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion:

Suggested change
for i, res_filter in enumerate(res):
# Add the field 'facet' to the result
res[i]["facet"] = filters.filtered(lambda f: f.id == res_filter["id"]).facet
for res_filter in res:
# Add the field 'facet' to the result
res_filter["facet"] = filters.filtered(lambda f: f.id == res_filter["id"]).facet

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Comment on lines 18 to 19
"web_edit_user_filter/static/src/xml/backend.xml",
"web_edit_user_filter/static/src/xml/search_extended.xml",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

chore: for consistence,

Suggested change
"web_edit_user_filter/static/src/xml/backend.xml",
"web_edit_user_filter/static/src/xml/search_extended.xml",
"web_edit_user_filter/static/src/xml/*.xml",

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done

Comment on lines 19 to 21
var searchItems = this.env.searchModel.getSearchItems(
(s) => s.type === "favorite"
);
const favorites = searchItems.filter(
(f) => f.description === this.state.description
);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

suggestion:

Suggested change
var searchItems = this.env.searchModel.getSearchItems(
(s) => s.type === "favorite"
);
const favorites = searchItems.filter(
(f) => f.description === this.state.description
);
var searchItems = this.env.searchModel.getSearchItems(
(s) => s.type === "favorite" && s.description === this.state.description
);

(there are a few similar simplifications that could be made around the code)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Modified as suggested. Identified in few other places and modified it. However, could not modify in places like here and popover is not visible when modifying it.

@aleuffre
Copy link
Contributor

aleuffre commented Dec 3, 2024

Unverified Scenarios:

  1. In unpack_filter function, there is a branch deals with tentative filter/groupBy which could not be verified as not able to understand the feature from README file or in general.
    So, I would like to know how this scenario could be verified? Any suggestions would be helpful to verify it.

From testing on v14, tentativeFilter (here and here) is used when customized filters are used via the "Add custom filter" and "Add custom group" buttons. That's when the pre-defined filter cannot be found. In that case, the module tries to compensate for it and find a similar filter, but it doesn't seem to me like it works very well at all (in v14).

In fact, in v14, contacts view:

  1. Group by Country
  2. Add Custom Group -> City (not in the pre-defined group-by options)
  3. Save as favourite, and reload the page
  4. Apply the filter, and then click on the pencil to edit it
  5. ISSUE: The filter is taken apart but only the Country groupby remains, instead of having the two nested filters like before.

I'd even go as far as suggesting removing that code path entirely, and adding to the Roadmap that the module is not currently compatible with custom filters. I don't think it's within the scope of the migration to fix that problem, necessarily.

Though of course the last word is up to a maintainer.

@anusriNPS anusriNPS force-pushed the 16.0-mig-web_edit_user_filter branch from 8643376 to f01d632 Compare December 4, 2024 11:53
@anusriNPS
Copy link
Contributor Author

Tried to use custom groups and filters as mentioned and it works in v16 and it is able to unpack the filters/groupBy which are selected from Add custom filter/group

For eg: (Both "Application" and "Created By" custom groups under App module)

  1. Click on "Add Custom Group -> "Application"
  2. Click on "Add Custom Group -> "Created By"
  3. Save as favorite.
  4. Click on favorite facet and then clicking on pencil icon, it unpacks selected custom groups.

However, for this case as well it did not go inside tentative filter related branch code, so removed for now from the PR.

In order to include as part of ROADMAP, I am not sure what to be mentioned as above custom related filters/groupBy works in v16(though it does not work in v14).

As part of latest patchset, corrected issue with displaying facet values and unpacking facets that related to type "field".

@aleuffre
Copy link
Contributor

aleuffre commented Dec 4, 2024

Tried to use custom groups and filters as mentioned and it works in v16 and it is able to unpack the filters/groupBy which are selected from Add custom filter/group

For eg: (Both "Application" and "Created By" custom groups under App module)

1. Click on "Add Custom Group -> "Application"

2. Click on "Add Custom Group -> "Created By"

3. Save as favorite.

4. Click on favorite facet and then clicking on pencil icon, it unpacks selected custom groups.

However, for this case as well it did not go inside tentative filter related branch code, so removed for now from the PR.

In order to include as part of ROADMAP, I am not sure what to be mentioned as above custom related filters/groupBy works in v16(though it does not work in v14).

As part of latest patchset, corrected issue with displaying facet values and unpacking facets that related to type "field".

I had not tested on v16, if it works correctly then great, no need to mention anything on the roadmap. Thank you

Copy link
Contributor

@aleuffre aleuffre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Code review, LGTM, just some minor non-blocking code cleanup possible

Comment on lines 39 to 41
var currentFacet = this.env.searchModel.getSearchItems(
(f) => f.type === facet_type && f.groupId == facetId
);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

comment (non-blocking): I think in this case using searchItems.filter was better, because you're filtering a subset of the searchItems you had already filtered beforehand, instead of searching from every filter in the model.

On the other hand, if searchItems is already filtered by facet_type and you use .filter here, you can drop the f.type === facet_type condition because it was already checked earlier.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated as suggested

@anusriNPS anusriNPS force-pushed the 16.0-mig-web_edit_user_filter branch from f01d632 to 9b842b6 Compare December 6, 2024 09:54
@anusriNPS
Copy link
Contributor Author

Identified an item for ROADMAP and included it on the ROADMAP file.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.